Skip to content

Conversation

@TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Nov 18, 2025

Patching vendor yields many problems. This PR removes last non-stmts-aware patch of php-parser

It basically integrates this https://raw.githubusercontent.com/rectorphp/vendor-patches/main/patches/nikic-php-parser-lib-phpparser-nodetraverser-php.patch into our code. Without any patching or manual managing the NodeVisitor changes 👍


How to run

php scripts/create-immutable-node-visitor.php 

What it does?

  • it creates src/PhpParser/NodeTraverser/AbstractImmutableNodeTraverser.php file that is identical to NodeTraverser with one difference - it fetches node visitors via getVisitorsForNode() abstract method
  • extend it and implement getVisitorsForNode() with propper caching - no need to get all 500 rules for Class_ node, maybe only those that match getNodeTypes() with Class_
  • test, optimize and profit 👍

Builds on: #6232 by @carlos-granados

@TomasVotruba TomasVotruba changed the title tv parent node traverser [internal] Extract ImmutableNodeTraverser from vendor, to remove patch and keep using cache for node types Nov 18, 2025
@TomasVotruba TomasVotruba force-pushed the tv-parent-node-traverser branch 2 times, most recently from 6c6ff65 to 3999e3e Compare November 19, 2025 00:34
@TomasVotruba TomasVotruba force-pushed the tv-parent-node-traverser branch from a0ff668 to 22d8f5f Compare November 19, 2025 10:17
@TomasVotruba TomasVotruba force-pushed the tv-parent-node-traverser branch from 22d8f5f to c3f4873 Compare November 19, 2025 10:19
@TomasVotruba
Copy link
Member Author

cc @carlos-granados Hi Carlos, you might like this one... I'm making the immutable traverser part of the Rector core, so we have it without vendor patching 👍 Thanks for your innovation and work to make it happen 🚀

@TomasVotruba
Copy link
Member Author

cc @staabm This is the immutable node traverser idea in new ImmutableNodeTraverser class. We saw 20-30 % performance improvements across various projects. I think PHPStan would benefit from this approach too. Tip for your next performance bottleneck hunting :)

@TomasVotruba
Copy link
Member Author

Let's ship this to test in real projects 🚀

@TomasVotruba TomasVotruba merged commit 9e7f77d into main Nov 19, 2025
54 checks passed
@TomasVotruba TomasVotruba deleted the tv-parent-node-traverser branch November 19, 2025 10:32
@carlos-granados
Copy link
Contributor

@TomasVotruba A very interesting take. Only problem I can see is if somehow Nikic's PhpParser greatly modified their own parser, but I don't really see that happening

@TomasVotruba
Copy link
Member Author

@carlos-granados Thanks for feedback and welcome back 👍 (still travelling?)

I was checking, and past ~3 years only one constant was added. I think we can handle the update + make the node traverser even faster for our purposes with similar caching you've introduced.

@staabm
Copy link
Contributor

staabm commented Dec 3, 2025

huhu,

This is the immutable node traverser idea in new ImmutableNodeTraverser class. We saw 20-30 % performance improvements across various projects. I think PHPStan would benefit from this approach too. Tip for your next performance bottleneck hunting :)

@TomasVotruba I looked thru the PR, but was not able to pin-point what in this PR makes it faster compared to before this PR.

or does this PR implement the optimization which was already done in #6232 just in a different way?

IIRC phpstan is already doing something similar: see #6232 (comment)

@TomasVotruba
Copy link
Member Author

@staabm Yes, it's same as #6232 , just without vendor patching. I pinged you now because I know vendor patching is very risky business (broken by PHPUnit recently), so would not recommend it.

This PR implements the same logic that is easy to reproduce without touching vendor 👍

As for PHPStan, lazy registry saves some performance. Not sure how it's implemented on traverser level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants